From b61d5a9a76363fad9cd10ac1c40288e01d43d85f Mon Sep 17 00:00:00 2001 From: Wei Liu Date: Thu, 4 Sep 2014 23:43:09 +0100 Subject: [PATCH] libxl: properly lock userdata store Originally libxl user data store didn't have lock at all. There could be such race condition as mentioned by Ian Jackson: Task 1 Task 2 Creating the domain Trying to shut down actually create domain observe domid start domain destruction delete all userdata destroy domain store the userdata *** forbidden state created: userdata exists but domain doesn't *** userdata has been leaked [ would now bomb out ] This patch adds in proper locking to libxl user data store. The lock is associated with a specific domain (i.e. a per-domain lock). As for locking hierachy, we first take CTX lock (which is implemented with pthread recursive mutex so even if the application has taken it we're fine), then take the file lock. These locks are released in reversed order. A new libxl error code ERROR_LOCK_FAIL is introduced to describe failure to acquire locks. Also factor out libxl__userdata_{retrieve,store}, so that other functions that already hold the lock can call them to manipulate user data. Signed-off-by: Wei Liu Acked-by: Ian Campbell --- tools/libxl/libxl.c | 8 ++++ tools/libxl/libxl_dom.c | 73 ++++++++++++++++++++++++++++++------ tools/libxl/libxl_internal.h | 10 +++++ tools/libxl/libxl_types.idl | 1 + 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 2ae5fcaa7a..9bb1a90bbe 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1530,6 +1530,7 @@ static void devices_destroy_cb(libxl__egc *egc, uint32_t domid = dis->domid; char *dom_path; char *vm_path; + libxl__carefd *lock = NULL; dom_path = libxl__xs_get_dompath(gc, domid); if (!dom_path) { @@ -1555,6 +1556,12 @@ static void devices_destroy_cb(libxl__egc *egc, xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/hvmloader", domid)); + /* This is async operation, we already hold CTX lock */ + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } libxl__userdata_destroyall(gc, domid); rc = xc_domain_destroy(ctx->xch, domid); @@ -1566,6 +1573,7 @@ static void devices_destroy_cb(libxl__egc *egc, rc = 0; out: + if (lock) libxl__unlock_domain_userdata(lock); dis->callback(egc, dis, rc); return; } diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index 5e0cb80f81..0dfdb088d5 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -1865,11 +1865,10 @@ out: return; } -int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid, - const char *userdata_userid, - const uint8_t *data, int datalen) +int libxl__userdata_store(libxl__gc *gc, uint32_t domid, + const char *userdata_userid, + const uint8_t *data, int datalen) { - GC_INIT(ctx); const char *filename; const char *newfilename; int e, rc; @@ -1898,7 +1897,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid, if (fd < 0) goto err; - if (libxl_write_exactly(ctx, fd, data, datalen, "userdata", newfilename)) + if (libxl_write_exactly(CTX, fd, data, datalen, "userdata", newfilename)) goto err; if (close(fd) < 0) { @@ -1920,18 +1919,42 @@ err: } if (rc) - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s for %s", + LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR, "cannot write/rename %s for %s", newfilename, filename); out: - GC_FREE; return rc; } -int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid, - const char *userdata_userid, - uint8_t **data_r, int *datalen_r) +int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid, + const char *userdata_userid, + const uint8_t *data, int datalen) { GC_INIT(ctx); + int rc; + libxl__carefd *lock; + + CTX_LOCK; + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__userdata_store(gc, domid, userdata_userid, + data, datalen); + + libxl__unlock_domain_userdata(lock); + +out: + CTX_UNLOCK; + GC_FREE; + return rc; +} + +int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid, + const char *userdata_userid, + uint8_t **data_r, int *datalen_r) +{ const char *filename; int e, rc; int datalen = 0; @@ -1943,13 +1966,13 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid, goto out; } - e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen); + e = libxl_read_file_contents(CTX, filename, data_r ? &data : 0, &datalen); if (e && errno != ENOENT) { rc = ERROR_FAIL; goto out; } if (!e && !datalen) { - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "userdata file %s is empty", filename); + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "userdata file %s is empty", filename); if (data_r) assert(!*data_r); rc = ERROR_FAIL; goto out; @@ -1958,7 +1981,33 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid, if (data_r) *data_r = data; if (datalen_r) *datalen_r = datalen; rc = 0; + +out: + return rc; +} + +int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid, + const char *userdata_userid, + uint8_t **data_r, int *datalen_r) +{ + GC_INIT(ctx); + int rc; + libxl__carefd *lock; + + CTX_LOCK; + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__userdata_retrieve(gc, domid, userdata_userid, + data_r, datalen_r); + + + libxl__unlock_domain_userdata(lock); out: + CTX_UNLOCK; GC_FREE; return rc; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 0cedd3092c..bc89e79e87 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -997,6 +997,16 @@ _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid, const char *userdata_userid, const char *wh); _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid); +/* Caller must hold userdata store lock before calling + * libxl__userdata_{retrieve,store} + * See libxl__{un,}lock_domain_userdata. + */ +_hidden int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid, + const char *userdata_userid, + uint8_t **data_r, int *datalen_r); +_hidden int libxl__userdata_store(libxl__gc *gc, uint32_t domid, + const char *userdata_userid, + const uint8_t *data, int datalen); _hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid, int suspend_cancel); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 931c9e96d0..95681d5c91 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -58,6 +58,7 @@ libxl_error = Enumeration("error", [ (-12, "OSEVENT_REG_FAIL"), (-13, "BUFFERFULL"), (-14, "UNKNOWN_CHILD"), + (-15, "LOCK_FAIL"), ], value_namespace = "") libxl_domain_type = Enumeration("domain_type", [ -- 2.30.2